Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OAuth application flow for builtin mobile app #15541

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

oliverguenther
Copy link
Member

@oliverguenther oliverguenther force-pushed the feature/53620/builtin-oauth-app-mobile branch 2 times, most recently from f17ce22 to 0b5ef1d Compare May 16, 2024 08:50
@oliverguenther oliverguenther marked this pull request as ready for review May 16, 2024 08:50
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general direction is looking fine functionality and code wise. I like that the PR applies the structure of services/contracts to this part of the application as well.

Apart from what I noted down next to the code, documenting the feature is a necessity.

app/seeders/oauth_applications_seeder.rb Outdated Show resolved Hide resolved
app/components/oauth/applications/row_component.rb Outdated Show resolved Hide resolved
Copy link
Member

@wielinde wielinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @oliverguenther thank you for this great and long awaited feature.

Disclaimer: I didn't check the code out. I simply read over the code changes.

One important thing that seems to be missing is the handling for existing tokens after the application got deactivated. I guess you have two options:

  • have some sort of danger zone before the deactivation and tell the admin that this will delete all existing tokens. That is rather cheap to implement.
  • check every API request, if the access token belongs to an OAuth application that is not deactive. That is expensive and probably not worth the effort.

Further, from the product perspective I see that client pretty generic and not mobile restricted. Thus I proposed a couple of changes (application name, redirect URI, ID).

While we are at it: Ideally we should tell client developers to implement "PKCE" to pretect the client from being intercepted by another app registering for the same redirect URI protocol. However, that is not in the responsibility of the OAuth provider, thus not strictly part of this PR.

Copy link
Contributor

@mereghost mereghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just reiterate @wielinde comments about security and overall approach.

app/contracts/oauth/applications/base_contract.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/seeders/oauth_applications_seeder.rb Show resolved Hide resolved
@Kharonus Kharonus force-pushed the feature/53620/builtin-oauth-app-mobile branch from 0b5ef1d to 9bc91f4 Compare August 29, 2024 11:16
@Kharonus Kharonus self-assigned this Aug 29, 2024
@Kharonus Kharonus force-pushed the feature/53620/builtin-oauth-app-mobile branch 2 times, most recently from 6cf1427 to f65f697 Compare September 6, 2024 12:26
@Kharonus Kharonus force-pushed the feature/53620/builtin-oauth-app-mobile branch from f65f697 to 44f21ee Compare September 11, 2024 08:45
@Kharonus Kharonus force-pushed the feature/53620/builtin-oauth-app-mobile branch from 3a4b40d to 820c4ab Compare September 12, 2024 11:27
Copy link
Contributor

@mereghost mereghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a ton. Code looks sane for the intended purposes.

@Kharonus Kharonus force-pushed the feature/53620/builtin-oauth-app-mobile branch from c40f870 to cf4c7c3 Compare September 13, 2024 11:30
@Kharonus Kharonus merged commit 28d8b74 into dev Sep 13, 2024
11 checks passed
@Kharonus Kharonus deleted the feature/53620/builtin-oauth-app-mobile branch September 13, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

6 participants